Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add default exportConditions for both jsdom and node environments #11924

Merged
merged 10 commits into from
Feb 9, 2022
Merged

feat: add default exportConditions for both jsdom and node environments #11924

merged 10 commits into from
Feb 9, 2022

Conversation

piranna
Copy link
Contributor

@piranna piranna commented Oct 2, 2021

Summary

#11863 adds support for a new optional exportConditions() method in custom environments, so it's possible to add extra conditions. In its tests, it add as an example the browser and node conditions to custom environments that extends from jest-environment-jsdom and jest-environment-node respectively. In normal execution, node extra condition is provided by default by Node.js itself, and although not provided by Node.js, browser is one of the Node.js endorsed ones.

This pull-request modify the jest-environment-jsdom and jest-environment-node environments to provide the browser and node conditions by default.

Test plan

This code is a copy&past of the actual one in the tests, so instead of being used in ad-hoc child classes on the tests or for demo purposses, they are provided as default values for everybody to use.

As a side note, with the PR now that ad-hoc classes could be removed and use the unmodified jest-environment-jsdom and jest-environment-node that this PR provides, so I can update the PR to remove them too, if you want.

@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2021

Codecov Report

Merging #11924 (ef89ae8) into main (a9b7e8a) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11924      +/-   ##
==========================================
- Coverage   67.25%   67.24%   -0.02%     
==========================================
  Files         329      329              
  Lines       17353    17357       +4     
  Branches     5074     5074              
==========================================
  Hits        11671    11671              
- Misses       5650     5654       +4     
  Partials       32       32              
Impacted Files Coverage Δ
packages/jest-environment-jsdom/src/index.ts 72.13% <0.00%> (-2.45%) ⬇️
packages/jest-environment-node/src/index.ts 70.49% <0.00%> (-2.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9b7e8a...ef89ae8. Read the comment docs.

@piranna
Copy link
Contributor Author

piranna commented Oct 3, 2021

I've removed the custom environment classes from tests to use the new ones that already provides the conditions, so this way they are actually being tested and code coverage doesn't decrease, and also it's removed duplicated code.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! This isn't safe to do as node means "anything node can load" which means .node or .json files (which isn't supported by ESM) and ESM files (which isn't supported by require). You can make this decision in your own project, but Jest itself cannot as it depends on user config.

We can probably add a way to configure this so the conditions can be provided by config, but just setting them by default won't work.

browser is safer, but would create a weird asymmetry imo

CHANGELOG.md Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Oct 15, 2021

Actually, due to #11919 (comment) we might need to do this. Still needs to go in a major release tho

@piranna
Copy link
Contributor Author

piranna commented Oct 15, 2021

This isn't safe to do as node means "anything node can load" which means .node or .json files (which isn't supported by ESM) and ESM files (which isn't supported by require).

But, that's exactly the purposse of node export condition, to load "anything node can load", no matter it's format... Also, according to Node.js docs, node seems to be used only to load CommonJS or ESM modules, and for .node files you need to use node-addon condition, that I've not added it in this PR but probably we should for completeness. In any case, they "should" be defined in the exports field after import and require entries, so since they are resolved in order by the spec, jest would never get there, and if so, you can have nested conditions to later explicit set if you want a require or an import.

According to Node.js docs, this is the expected behaviour and usage, so I think it's totally save to add this PR functionality.

@piranna
Copy link
Contributor Author

piranna commented Oct 15, 2021

Still needs to go in a major release tho

No issues on that, fine for me :-)

@piranna
Copy link
Contributor Author

piranna commented Nov 14, 2021

Is this ready to be merged for the upcoming Jest 28?

@piranna
Copy link
Contributor Author

piranna commented Jan 3, 2022

Any update on this? What needs to be done so it gets merged? It has lasted two months now...

@piranna
Copy link
Contributor Author

piranna commented Jan 3, 2022

Should node-addon condition be added too?

@piranna
Copy link
Contributor Author

piranna commented Jan 17, 2022

Any update on this? It seems like k-g-a/jest-node-exports-resolver#9 is directly caused by the lacking of node condition, that this PR fixes... How can I help to get it merged?

@piranna
Copy link
Contributor Author

piranna commented Jan 26, 2022

Any update on this? How can we ver It merged?

@SimenB
Copy link
Member

SimenB commented Jan 27, 2022

when we start landing breaking changes

@SimenB SimenB added this to the Jest 28 milestone Jan 27, 2022
@piranna piranna requested a review from SimenB January 28, 2022 06:22
CHANGELOG.md Outdated Show resolved Hide resolved
@piranna
Copy link
Contributor Author

piranna commented Jan 28, 2022

Tests are passing except for a Windows odd one, seems to be a random failure unrelated to this PR. Otherwise, this is ready to be merged.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! I'll land this when I start landing breaking changes (hopefully next week)

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet, thanks!

@SimenB
Copy link
Member

SimenB commented Feb 7, 2022

I'm not allowed to make changes to this PR btw, so when the time comes to merge it, I'll need you to resolve conflicts. If you can give me access (https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork) I can handle any conflicts myself 🙂

@piranna
Copy link
Contributor Author

piranna commented Feb 7, 2022

I'm not allowed to make changes to this PR btw, so when the time comes to merge it, I'll need you to resolve conflicts. If you can give me access (docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork) I can handle any conflicts myself

It doesn't shows me that option, maybe are they only for maintainers? It shows you are only as contributor... Anyway, no problem on do the conflicts resolving myself, just ping me :-)

@SimenB
Copy link
Member

SimenB commented Feb 7, 2022

I'm allowed in essentially every single PR, don't think I've experienced not being able to outside of deleted branches/forks.

Probably because you've opened the PR from the main branch instead of branching off

@piranna
Copy link
Contributor Author

piranna commented Feb 7, 2022

Probably because you've opened the PR from the main branch instead of branching off

It would made sense. Anyway, just ping me and I'll do any needed changes, no issues :-)

@SimenB
Copy link
Member

SimenB commented Feb 7, 2022

Cool 👍 I'll start landing breaking changes at some point this week

@piranna
Copy link
Contributor Author

piranna commented Feb 7, 2022

Cool I'll start landing breaking changes at some point this week

Great :-)

@SimenB
Copy link
Member

SimenB commented Feb 8, 2022

@piranna if you rebase now, I think it should be good to land tomorrow 🙂 No current plans to land any features before this, so in theory no conflicts

@piranna
Copy link
Contributor Author

piranna commented Feb 8, 2022

@piranna if you rebase now, I think it should be good to land tomorrow No current plans to land any features before this, so in theory no conflicts

Rebase, or just fix conflicts? I've just done the second one.

@SimenB
Copy link
Member

SimenB commented Feb 8, 2022

Yeah, whatever is easiest for you 🙂 Merging is fine! We squash on merge anyways, so doesn't really matter how messy the commit history on the branch is 😀

@piranna
Copy link
Contributor Author

piranna commented Feb 8, 2022

Then you already have it :-)

@SimenB SimenB changed the title Add default exportConditions for both jsdom and node environments feat: add default exportConditions for both jsdom and node environments Feb 9, 2022
@SimenB SimenB merged commit 432dfb6 into jestjs:main Feb 9, 2022
@SimenB
Copy link
Member

SimenB commented Feb 9, 2022

Thanks!

@piranna
Copy link
Contributor Author

piranna commented Feb 9, 2022

Welcome! :-D

@SimenB
Copy link
Member

SimenB commented Feb 10, 2022

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants